Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update packages to make publish smaller #49

Merged
merged 4 commits into from
Mar 29, 2024

Conversation

leangseu-edx
Copy link
Contributor

@leangseu-edx leangseu-edx commented Mar 21, 2024

  • update dependency and peer dependency for the project.
  • replace @edx/frontend-plugin-framework with @openedx/frontend-plugin-framework
  • update example to use the latest compiled code whether is from npm start or npm run build
  • add script to run example with npm run start:example

@leangseu-edx leangseu-edx requested review from arbrandes and jsnwesson and removed request for arbrandes March 21, 2024 18:04
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (bf2f1bf) to head (c9b8fa3).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #49   +/-   ##
=======================================
  Coverage   96.53%   96.53%           
=======================================
  Files          10       10           
  Lines         173      173           
  Branches       35       35           
=======================================
  Hits          167      167           
  Misses          5        5           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package.json Outdated Show resolved Hide resolved
Copy link
Member

@MaxFrank13 MaxFrank13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jason and I were just talking about this! Thanks for doing this work. I had one comment but it's non-blocking

@leangseu-edx
Copy link
Contributor Author

I just realized that I do not have permission to merge this pr. @jsnwesson can you merge after review it?

@jsnwesson
Copy link
Contributor

Hey @leangseu-edx ! Thanks for looking into this, although I noticed that when I tried running the "host" example app, it wasn't able to render the plugins. Having compared it to the master branch, I can confirm that some dependency in this PR is causing those plugins to go missing. Would you be able to look into it?

Screenshot 2024-03-26 at 8 55 25 AM

@leangseu-edx
Copy link
Contributor Author

Hey @leangseu-edx ! Thanks for looking into this, although I noticed that when I tried running the "host" example app, it wasn't able to render the plugins. Having compared it to the master branch, I can confirm that some dependency in this PR is causing those plugins to go missing. Would you be able to look into it?

I found the issue. It seems like

    "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2",
    "@edx/frontend-component-footer": "13.0.3",
    "@edx/frontend-component-header": "5.0.2",

are required at the root of the project. I don't know why it seems necessary.

Additional file changed are just replacing @edx/frontend-plugin-framework with @openedx/frontend-plugin-framework.

@leangseu-edx
Copy link
Contributor Author

leangseu-edx commented Mar 26, 2024

Sorry to add more stuff to this pr. I just feel like it would help with future development. I updated the Using the Example Apps section on README. I believe the last pr should help people develop the application better with hot reload on all subproject and the main project library.

@jsnwesson
Copy link
Contributor

@leangseu-edx no apologies needed, this is all greatly appreciated given the time you've put into making the experience cleaner!
One thing to note is that there looks to be a mix up of port numbers in the changes you made so far, but there's also other parts of the example files that likely need to be synced with the correct port number, as I wasn't able to see any of the iFrame-based plugins in the host MFE.

To make sure the terms are clear:

  • "Host MFE" AKA the example app

    • Original PORT was 8080 according to the JS-config values, BASE_URL and PORT
    • Now looks to be 8081 according to webpack.dev.config.js
  • "Child MFE" AKA the example-plugin-app

    • Original PORT was 8081 according to the JS-config values BASE_URL and PORT and the customization in webpack.dev.config.js
    • Now looks to be 8082 according to webpack.dev.config.js

Oddly enough, I was able to see the Host MFE by going to localhost:8082, which according to your changes should've rendered the Child MFE instead. Not sure why that is.
Additionally, I noticed in the README "Getting Started" section that devs should go to localhost:8080 to see the example app, but perhaps it was meant to point to whatever the port is for Host MFE?

@leangseu-edx
Copy link
Contributor Author

leangseu-edx commented Mar 27, 2024

To make sure the terms are clear:

  • "Host MFE" AKA the example app

    • Original PORT was 8080 according to the JS-config values, BASE_URL and PORT
    • Now looks to be 8081 according to webpack.dev.config.js
  • "Child MFE" AKA the example-plugin-app

    • Original PORT was 8081 according to the JS-config values BASE_URL and PORT and the customization in webpack.dev.config.js
    • Now looks to be 8082 according to webpack.dev.config.js

Oddly enough, I was able to see the Host MFE by going to localhost:8082, which according to your changes should've rendered the Child MFE instead. Not sure why that is. Additionally, I noticed in the README "Getting Started" section that devs should go to localhost:8080 to see the example app, but perhaps it was meant to point to whatever the port is for Host MFE?

I was sure I changed that back yesterday. At first I thought src need a port to watch the file change and hot reload. Which was the reason I tried to put it at 8080 and the rest got bump.

Anyway, now it should changed accordingly.
src use babel to transpile with watch
example run on 8080
example-plugin-app run on 8081

@jsnwesson
Copy link
Contributor

Sweeet thanks for the fix! Approved and greatly appreciated. Thanks again @leangseu-edx !

@jsnwesson jsnwesson merged commit 9be2cd4 into openedx:master Mar 29, 2024
7 checks passed
@openedx-semantic-release-bot

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@leangseu-edx leangseu-edx deleted the lk/update-dependency branch April 2, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants